Skip to content

Conversation

@christian-oreilly
Copy link
Collaborator

@christian-oreilly christian-oreilly commented Dec 11, 2024

Fix #104
Fix #109

@christian-oreilly christian-oreilly linked an issue Dec 11, 2024 that may be closed by this pull request
3 tasks
@christian-oreilly christian-oreilly marked this pull request as draft December 11, 2024 16:43
@christian-oreilly
Copy link
Collaborator Author

This PR will wait to be rebased after #80 merges before further testing to avoid having to solve conflicts in notebooks due to the pre-commit hook.

@christian-oreilly christian-oreilly marked this pull request as ready for review January 29, 2025 17:21
@christian-oreilly christian-oreilly self-assigned this Jan 29, 2025
@christian-oreilly christian-oreilly added documentation Improvements or additions to documentation DevOps labels Jan 29, 2025
@christian-oreilly
Copy link
Collaborator Author

@hanayik I sent you an email at your ndcn.ox.ac.uk address but I am not sure if you received it. If not, let me know. It is about adding a codecov token to the ipyniivue project. I am not part of the NiiVue organization so I cannot do it myself.

@christian-oreilly
Copy link
Collaborator Author

@bcalford That's cool. The CI is now running the notebooks and the tests flag any crashing notebook. It allowed us to find that two of our notebooks were broken! One was just missing an import, but the other was due to a change in the API. This will make it easier for us to maintain everything in working order!

@hanayik
Copy link
Member

hanayik commented Jan 30, 2025

@christian-oreilly I received the email, thanks! We can keep the conversation on here though since it's in context with the PR.

I don't think it's necessary to give the coverage service access to the entire Niivue github organisation.

I have instead modified the CI workflow to use pytest-cov and upload the coverage summary as artifacts and show the coverage as markdown tables in the workflow summary page.

see: https://github.com/niivue/ipyniivue/actions/runs/13050900236?pr=105

Let me know if this is ok for your use case. The first table is the src code summary, and the second table is the notebook coverage summary (per matrix combination in your test runners).

@christian-oreilly christian-oreilly mentioned this pull request Jan 30, 2025
@christian-oreilly
Copy link
Collaborator Author

@hanayik Thank for your feedback. I am not sure I get where this report is uploaded. I relaunched the jobs to check the result I don't see the report added to the PR. I am trying to have the report added to the PR as can be seen in this PR lina-usc/pylossless#214 for example. My understanding is that it is done through this

https://github.com/lina-usc/pylossless/blob/d835e404b5a213eea1352e4fcb200e2f631cd480/.github/workflows/test_pipeline.yml#L46-L48

which requires a token for codecov. I does not require giving the coverage access to the whole organization, just to ipyniivue repo. I would have done it myself, but I can't because I am not a member of the NiiVue organization.

@christian-oreilly
Copy link
Collaborator Author

Thanks for the review @bcalford

@christian-oreilly christian-oreilly merged commit 7812824 into main Feb 5, 2025
19 checks passed
@christian-oreilly christian-oreilly deleted the 104-adding-testing-to-ci branch February 5, 2025 22:03
@bcalford bcalford mentioned this pull request Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DevOps documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Every public class, function, class method, and class attribute has a doc string following pydocstyle Adding testing to CI

4 participants